sstable: add block level synthetic prefix support #3237
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 16 files reviewed, 3 unresolved discussions (waiting on @dt, @msbutler, and @petermattis)
sstable/block.go line 414 at r1 (raw file):
} hideObsoletePoints bool SyntheticPrefix
[nit] we should avoid embedding unless we want to specifically inherit the methods
sstable/block.go line 644 at r1 (raw file):
return base.CorruptionErrorf("pebble/table: invalid firstKey in block") } if i.SyntheticPrefix != nil {
Should we use a reusable buffer (similar to fullKey) to avoid this alloc when restForReuse is used?
sstable/reader.go line 174 at r1 (raw file):
} type SyntheticPrefix []byte
[nit] will need some comments
This adds the concept of synthetic prefixes to sstable readers and their underlying block iterators, treating the synthetic prefix as an extra shared prefix, shared even by restart keys. When a reader is configured with a synthetic prefix, it will assume that that prefix is implicitly prepended to every key in the underlying sst blocks when interacting with or returning those keys.
There was a problem hiding this comment.
Reviewable status: 0 of 16 files reviewed, 8 unresolved discussions (waiting on @dt and @petermattis)
sstable/block.go line 647 at r2 (raw file):
} if i.prefix != nil { i.firstUserKey = append(append(i.firstUserKeyWithPrefix[:0], i.prefix...), i.firstUserKey...)
nit: the naming here is confusing. I thought firstUserKey was without the prefix, but it seems it is also with prefix, and firstUserKeyWithPrefix is really firstUserKeyWithPrefixBacking.
sstable/block.go line 704 at r2 (raw file):
// SeekGE implements internalIterator.SeekGE, as documented in the pebble // package. func (i *blockIter) SeekGE(key []byte, flags base.SeekGEFlags) (*InternalKey, base.LazyValue) {
what's the effect of these changes on benchmarks that don't have a prefix?
sstable/block.go line 863 at r2 (raw file):
if i.prefix != nil { if !bytes.HasPrefix(key, i.prefix) { if i.cmp(i.prefix, key) < 0 {
where do we say that Compare needs to be defined for a prefix and not just user keys? For example EngineKeyCompare (the CRDB implementation of Compare) is explicitly looking for the separator between the prefix and the suffix.
|
There is something subtle going on here. We only cache physical SST readers. We are pushing down the prefix replacement rules from the virtual SST into the physical reader (which can be shared among all virtual SSTs). In principle we can currently have multiple virtual SSTs with the same backing SST that have different prefix replacement rules. I believe we don't need that functionality so we should require (and enforce) that all virtual SSTs with the same backing file have the same replacement rules. Once we do that, it's ok to push down the rules into the physical reader. (but we should still add an assertion that these things match when we use a cached physical reader). This is an existing issue w.r.t |
|
Previously, RaduBerinde wrote…
Sounds like we can't assume all virtual SSTs will have the same suffix replacement rules (we would like these to be plumbed together). Maybe we can pass these rules only when we create iterators (from VirtualReader)? |
|
moving pr to #3350 |
This adds the concept of synthetic prefixes to sstable readers and their underlying block iterators, treating the synthetic prefix as an extra shared prefix, shared even by restart keys.
When a reader is configured with a synthetic prefix, it will assume that that prefix is implicitly prepended to every key in the underlying sst blocks when interacting with or returning those keys.